-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mitigated most bugprone-unused-return-value
clang-tidy warnings in test code when we enable it for *all* functions
#6673
Conversation
See llvm/llvm-project#85913 for a discussion about the stream insertion operator "false positives". |
@@ -877,7 +877,7 @@ class TestVarID : public TestFixture { | |||
void varid36() { // ticket #2980 (segmentation fault) | |||
const char code[] ="#elif A\n" | |||
"A,a<b<x0\n"; | |||
tokenize(code); | |||
(void)tokenize(code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could also be written as ASSERT_NO_THROW()
and my changes here are as inconsistent as the existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just disallow using (void)
and that would force us to use ASSERT_NO_THROW
. I think I might do that in a follow-up PR to make things consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are also some unnecessary return values in the test code which could also be cleaned up in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed https://trac.cppcheck.net/ticket/13006 about detecting functions which always have their result ignored.
5e679fa
to
e00d83e
Compare
…test code when we enable it for *all* functions
be10d7e
to
a520374
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An observation I have is that I believe we don't want to have warnings about all unused return values in all our code.
99% of the fixes here seems to be "false positives" we are really not interested in the return value.. but there are some cases where it does make sense to check the result.
There is Misra C++ 2023 0.1.2 rule that also complains about this and I find it way too noisy to be used.
That's why I only apply it to the test code.
I think a lot of what I suppressed here has return codes for no good reason. I will explore that in a follow-up now that usage/suppression has been clarified by these fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I only apply it to the test code.
Good. I approve this with that limitation.
We should not have any unchecked returns within the test code to make sure everything is working as expected (there might be a case that should also apply to the production code).
Unfortunately enabling all functions is abusing this check and it will produce a lot of "false positives" (e.g.
erase()
/insert()
of containers or usage of stream insertion operators) so this cannot be enabled by default. Sincellvm:Regex
does not support negative lookahead we cannot configure the check to exclude functions.